Skip to content

Conversation

@kernel-patches-daemon-bpf-rc
Copy link

Pull request for series with
subject: Change a lot of min_t() that might mask high bits
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1025536

David Laight added 12 commits November 19, 2025 14:49
min_t() and max_t() are normally used to change the signedness
of a positive value to avoid a signed-v-unsigned compare warning.

However they are used here to convert an unsigned 64bit pattern
to a signed to a 32/64bit signed number.
To avoid any confusion use plain min()/max() and explicitely cast
the u64 expression to the correct signed value.

Use a simple max() for the max_pkt_offset calulation and delete the
comment about why the cast to u32 is safe.

Signed-off-by: David Laight <[email protected]>
There are some dodgy clamp_t(u16, ...) and min_t(u16, ...).

__skb_flow_dissect() tries to cap its return value with:
	key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
however this casts skb->len to u16 before the comparison.
While both nboff and hlen are 'small', skb->len could be 0x10001 which
gets converted to 1 by the cast.
This gives an invalid (small) value for thoff for valid packets.

bpf_flow_dissect() used clamp_t(u16, ...) to set both flow_keys->nhoff
and flow_keys->thoff.
While I think these can't lose significant bits the casts are unnecessary
plain clamp(...) works fine.

Fixes: d0c081b ("flow_dissector: properly cap thoff field")
Signed-off-by: David Laight <[email protected]>
In ethtool_cmis_cdb_execute_epl_cmd() change space_left and
bytes_to_write from u16 to u32.
Although the values may fit in 16 bits, 32bit variables will generate
better code.
Replace the nested min_t(u16, bytes_left, min_t(u16, space_left, x))
with a call to min3().

Signed-off-by: David Laight <[email protected]>
The implicit casts done by max_t() should only really be used to
convert positive values to signed or unsigned types.
In the EMSGSIZE error path
	pmtu = max_t(int, mtu - headersize + sizeof(struct ipv6hdr), 0);
is being used to convert a large unsigned value to a signed negative one.
Rework using a signed temporary variable and max(pmtu, 0), as well as
casting sizeof() to (int) - which is where the unsignedness comes from.

Signed-off-by: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'.
Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long'
and so cannot discard significant bits.

In this case the 'unsigned long' value is small enough that the result
is ok.

Detected by an extra check added to min_t().

Signed-off-by: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'.
Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long'
and so cannot discard significant bits.

In this case the 'unsigned long' value is small enough that the result
is ok.

Detected by an extra check added to min_t().

Signed-off-by: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'.
Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long'
and so cannot discard significant bits.

In this case the 'unsigned long' value is small enough that the result
is ok.

Detected by an extra check added to min_t().

Signed-off-by: David Laight <[email protected]>
It is invalid to use sizeof() or typeof() in bitfields which stops
them being passed to max().
This has been fixed by using max_t().
I want to add some checks to max_t() to detect cases where the cast
discards non-zero high bits - which uses sizeof().
So add 0 to the bitfield (converting it to int) then use max().

Signed-off-by: David Laight <[email protected]>
Loops like:
	int copied = ...;
	...
	while (copied) {
		use = min_t(type, copied, PAGE_SIZE - offset);
		...
		copied -= 0;
	}
can be converted to a plain min() if the comparison is changed to:
	while (copied > 0) {
This removes any chance of high bits being discded by min_t().
(In the case above PAGE_SIZE is 64bits so the 'int' cast is safe,
but there are plenty of cases where the check shows up bugs.)

Signed-off-by: David Laight <[email protected]>
min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'.
Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long'
and so cannot discard significant bits.

In this case the 'unsigned long' value is small enough that the result
is ok.

Detected by an extra check added to min_t().

Signed-off-by: David Laight <[email protected]>
The scan limit in genl_allocate_reserve_groups() is:
	min_t(int, id + n_groups, mc_groups_longs * BITS_PER_LONG);
While 'id' and 'n_groups' are both 'int', 'mc_groups_longs' is
'unsigned long' (BITS_PER_LONG is 'int').
These inconsistent types (all the values are small and non-negative)
means that a simple min() fails.

When checks for masking high bits are added to min_t() that also fails.
Instead use umin() so safely convert all the values to unsigned.

Move the limit calculation outside the loop for efficiency and
readability.

Signed-off-by: David Laight <[email protected]>
There are two:
	min_t(int, xxx, mptcp_wnd_end(msk) - msk->snd_nxt);
Both mptcp_wnd_end(msk) and msk->snd_nxt are u64, their difference
(aka the window size) might be limited to 32 bits - but that isn't
knowable from this code.
So checks being added to min_t() detect the potential discard of
significant bits.

Provided the 'avail_size' and return of mptcp_check_allowed_size()
are changed to an unsigned type (size_t matches the type the caller
uses) both min_t() can be changed to min().

Signed-off-by: David Laight <[email protected]>
@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: d6ec090
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1025536
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1025536 expired. Closing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant